Skip to content

Clarify multiple IV curve requirement for Sandia fitting methods - #2883

Open
anshurajbisoyi98-ctrl wants to merge 2 commits into
pvlib:mainfrom
anshurajbisoyi98-ctrl:docs-1715-multiple-iv-curves
Open

anshurajbisoyi98-ctrl wants to merge 2 commits into
pvlib:mainfrom
anshurajbisoyi98-ctrl:docs-1715-multiple-iv-curves

Conversation

@anshurajbisoyi98-ctrl

@anshurajbisoyi98-ctrl anshurajbisoyi98-ctrl commented Oct 4, 2026 •

Copy link
Copy Markdown

Both fitting-function docstrings describe per-curve arrays but do not explicitly say that multiple IV curves are required. This adds that requirement to their introductory descriptions and clarifies that each curve needs corresponding effective irradiance and cell temperature data.

Validation: 11 tests passed in the De Soto and PVsyst test modules (including both Sandia fitting tests). HTML documentation built with gallery execution disabled, and both rendered function pages were checked. The tests emit numerical/deprecation warnings; the documentation emits an existing configuration-cache warning.

  • Closes Clarify docstrings for ivtools.sdm.fit_pvsyst_sandia, .fit_desoto_sandia #1715
  • I am familiar with the contributing guidelines
  • I attest that all AI-generated material has been vetted for accuracy and is in compliance with the pvlib license
  • Tests added: not applicable; documentation-only change, existing tests run.
  • Reference entries: not applicable; no API changes.
  • Adds description and name entries in the appropriate "what's new" file, including the issue link and contributor username.
  • Updated docstrings follow the existing numpydoc format.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels (including remote-data where applicable) and Milestone are assigned to the Pull Request and linked Issue.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

Hey @anshurajbisoyi98-ctrl! 🎉

Thanks for opening your first pull request! We appreciate your
contribution. Please ensure you have reviewed and understood the
contributing guidelines.

If AI is used for any portion of this PR, you must vet the content
for technical accuracy.

Finally, be sure the PR description includes the PR
checklist,
and complete the items you are able to.

@cwhanse cwhanse left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need to include effective irradiance and cell temperature is clear with ee and tc in the ivcurves dict.

Comment thread pvlib/ivtools/sdm/desoto.py Outdated
Comment on lines +225 to +226
and cell temperature conditions. Each curve must have corresponding
effective irradiance and cell temperature data in ``ivcurves``.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
and cell temperature conditions. Each curve must have corresponding
effective irradiance and cell temperature data in ``ivcurves``.
and cell temperature conditions.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I removed the redundant sentence from both docstrings in commit d0c2a5e.

Comment thread pvlib/ivtools/sdm/pvsyst.py Outdated
Comment on lines +29 to +30
and cell temperature conditions. Each curve must have corresponding
effective irradiance and cell temperature data in ``ivcurves``.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
and cell temperature conditions. Each curve must have corresponding
effective irradiance and cell temperature data in ``ivcurves``.
and cell temperature conditions.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I removed the redundant sentence from both docstrings in commit d0c2a5e.

@cwhanse cwhanse added this to the v0.16.2 milestone Oct 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify docstrings for ivtools.sdm.fit_pvsyst_sandia, .fit_desoto_sandia

2 participants